Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Form object refactoring #11905

Merged
merged 81 commits into from
Feb 8, 2023
Merged

Conversation

volha-pivavarchyk
Copy link
Contributor

@volha-pivavarchyk volha-pivavarchyk commented Feb 1, 2023

Q A
Bug fix? (use the a.b branch) [ ]
New feature/enhancement? (use the a.x branch) [ ]
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [ ]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #...

Description:

This PR replaces PR #11234: rebased and errors fixes

Steps to test this PR:

  1. Open mautic/mautic on Gitpod or pull down for testing locally (see docs on testing PRs here) - as you need to do some things before applying the PR.

  2. Before you checkout this PR:

  3. Create a form with at least 4 fields. One mapped to the contact email field, one mapped to the company field under the contact opt group, one mapped to the primary company city field. The fourth field should not be mapped to any field.

  4. Check out this PR - in Gitpod on the left hand side, look for the Github icon, then find this PR and right click to checkout the PR:
    screenshot-mautic-mautic-oq31hzs9992 ws-eu85 gitpod io-2023 02 04-15_36_18

  5. Run the migration using the terminal tab.

Run ddev exec bin/console doctrine:migrations:execute --up 20200415135706. Note: you can roll back your changes with ddev exec bin/console doctrine:migrations:execute --down 20200415135706 if needed.

  1. Open PHPMyAdmin under the Ports tab on Gitpod, look for the port 8036. Open it in a tab.

  2. Open the database named 'db' and go to the form_fields table and check that the last 2 columns are mapped_object and mapped_field. The field that was mapped to the contact email field should have mapped_object = 'lead' and mapped_field = 'email'. The one that was mapped to the company field under the contact opt group should have mapped_object = 'lead' and mapped_field = 'company'. The one that was mapped to the primary company city field should have mapped_object = 'company' and mapped_field = 'companycity'. The form field that was not mapped at all should have null values there.

  3. When you map one field then it should be removed from mapping of other fields. Example: Map contact email field to a form field. Create another field. The contact email field is not in the mapping list.

  4. Test submissions of different field types.

  5. Test conditional fields

  6. Test progressive profiling (behaviours)

List deprecations along with the new alternative:

  1. form_fields.lead_field column is deprecated. Use mapped_object and mapped_field instead.
  2. Mautic\FormBundle\Entity\Field::getLeadField() and setLeadField(). Use getters and setters on mappedObject and mappedField properties instead.
  3. Mautic\FormBundle\Event\Service\FieldValueTransformer::isIsTransformed() is deprecated as not used.

together with migration that will create the columns and migrates data from lead_field column.

I cleaned the entity a bit too. It was full of verbose syntax, strings over constants and comments that did not bring any value.
by not extending FormTestAbstract
And shorter syntax is used to create mocks
plus removing inheritance from tests, stop booting kernel for unit tests, speeding up tests by 3 seconds.
@RCheesley RCheesley added the pending-test-confirmation PR's that require one test before they can be merged label Feb 4, 2023
@mabumusa1 mabumusa1 self-requested a review February 4, 2023 16:04
mabumusa1
mabumusa1 previously approved these changes Feb 4, 2023
Copy link
Member

@mabumusa1 mabumusa1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested locally, this PR is a great addition. It works as described

@RCheesley RCheesley added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels Feb 4, 2023
@RCheesley
Copy link
Sponsor Member

RTC assuming the comments from @escopecz on the code review are satisfied, which it looks like they are.

@volha-pivavarchyk
Copy link
Contributor Author

@RCheesley @escopecz @mabumusa1 I removed the commented code that was unnecessary. This code is not used and shouldn't affect anything. Could you please test and re-approve it again.

@mabumusa1 mabumusa1 self-requested a review February 8, 2023 07:21
Copy link
Member

@mabumusa1 mabumusa1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this PR again, after the removal of unused code

@RCheesley
Copy link
Sponsor Member

Thanks for all the testing folks, will commit this once the tests complete! 🚀

@RCheesley RCheesley merged commit bed5624 into mautic:5.x Feb 8, 2023
@RCheesley RCheesley added this to the 5.0-alpha milestone Feb 8, 2023
@RCheesley RCheesley added forms Anything related to forms refactoring The change does not change behavior but improves the code labels Feb 8, 2023
JoshuaEstes added a commit to JoshuaEstes/mautic that referenced this pull request Feb 8, 2023
* upstream/5.x:
  TPROD-334 - MauticCrmBundle - Migrate PHP Templates to Twig (mautic#11936)
  TPROD-333 - MauticClearbitBundle - Convert PHP Templates to Twig Templates (mautic#11937)
  Form object refactoring (mautic#11905)
  docs: add volha-pivavarchyk as a contributor for code (mautic#11940)
  quick smoke test
  TPROD-321 - NotificationBundle - Popup
JoshuaEstes added a commit to JoshuaEstes/mautic that referenced this pull request Feb 8, 2023
… import/TPROD-319

* 'import/TPROD-319' of github.com:JoshuaEstes/mautic: (33 commits)
  TPROD-334 - MauticCrmBundle - Migrate PHP Templates to Twig (mautic#11936)
  TPROD-333 - MauticClearbitBundle - Convert PHP Templates to Twig Templates (mautic#11937)
  Form object refactoring (mautic#11905)
  docs: add volha-pivavarchyk as a contributor for code (mautic#11940)
  finish templates
  TPROD-232 - MauticFullContactBundle - Convert PHP Templates to Twig Templates
  twig lint
  builder works
  test
  check
  check
  checkpoint
  categorylist slot
  bug fix, cleanup
  html validation
  form theme
  form theme template
  cleanup
  cleanup
  make sure builder uses twig templates instead of php
  ...
JoshuaEstes added a commit to JoshuaEstes/mautic that referenced this pull request Feb 9, 2023
* upstream/5.x: (37 commits)
  TPROD-334 - MauticCrmBundle - Migrate PHP Templates to Twig (mautic#11936)
  TPROD-333 - MauticClearbitBundle - Convert PHP Templates to Twig Templates (mautic#11937)
  Form object refactoring (mautic#11905)
  docs: add volha-pivavarchyk as a contributor for code (mautic#11940)
  All contributors/add lord rembo (mautic#11935)
  finish templates
  TPROD-232 - MauticFullContactBundle - Convert PHP Templates to Twig Templates
  Update UPGRADE-PHP-TO-TWIG-TEMPLATES.md (mautic#11921)
  twig lint
  builder works
  test
  check
  check
  checkpoint
  categorylist slot
  bug fix, cleanup
  TPROD-319 - LeadBundle - Custom Fields Templates (mautic#11819)
  html validation
  form theme
  form theme template
  ...
@escopecz escopecz added the bug Issues or PR's relating to bugs label Jun 23, 2023
@mautibot
Copy link

mautibot commented Jan 9, 2024

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/upgrade-from-v4-4-10-to-5-renders-forms-not-accessable-schema-issues/30415/4

@mautibot
Copy link

mautibot commented Jan 9, 2024

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/upgrade-from-v4-4-10-to-5-renders-forms-not-accessable-schema-issues/30415/5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs cla-signed The PR contributors have signed the contributors agreement code-review-needed PR's that require a code review before merging forms Anything related to forms ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged refactoring The change does not change behavior but improves the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants